chore(aggregated-assets): scaffold the DDD target packages (LIVE-35225) - #20285
Conversation
LucasWerey
changed the title
Web Tools Build Status
|
There was a problem hiding this comment.
🟡 Not ready to approve
The new platform package’s Jest setup is incomplete for jest-dom usage, and the lockfile diff includes non-scaffold-related metadata churn that should be minimized or explained.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR scaffolds new DDD workspaces to host the upcoming DADA (Dynamic Assets Data Aggregator) migration out of live-common, creating placeholder domain/entity, domain/api, and features/platform packages with initial tooling/config.
Changes:
- Add new empty DDD packages:
@domain/entity-asset,@domain/entity-interest-rate, and@domain/api-assets-data. - Add a new app-facing platform layer package
@features/platform-assets-data(currently scaffolded/empty). - Register the new workspaces in
knip.json, add a changeset, and update the lockfile for the new importers.
File summaries
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds new workspace importers for the scaffolded packages; also updates some existing package snapshot/peer metadata. |
| knip.json | Registers the new workspaces for knip entrypoints. |
| features/platform/assets-data/tsconfig.json | TypeScript config for the new platform package. |
| features/platform/assets-data/src/index.ts | Placeholder entrypoint. |
| features/platform/assets-data/README.md | Documents intended responsibilities and constraints of the platform layer. |
| features/platform/assets-data/project.json | Nx project scaffold (noop build). |
| features/platform/assets-data/package.json | Workspace manifest, scripts, and dependencies for the platform package. |
| features/platform/assets-data/jest.config.js | Jest config scaffold for the platform package. |
| domain/entity/interest-rate/tsconfig.json | TypeScript config for the new entity package. |
| domain/entity/interest-rate/src/index.ts | Placeholder entrypoint. |
| domain/entity/interest-rate/README.md | Documents intended scope and future migration source. |
| domain/entity/interest-rate/project.json | Nx project scaffold (noop build). |
| domain/entity/interest-rate/package.json | Workspace manifest and scripts for the interest-rate entity package. |
| domain/entity/interest-rate/jest.config.js | Jest config scaffold for the interest-rate entity package. |
| domain/entity/asset/tsconfig.json | TypeScript config for the new entity package. |
| domain/entity/asset/src/index.ts | Placeholder entrypoint. |
| domain/entity/asset/README.md | Documents intended scope and future migration source. |
| domain/entity/asset/project.json | Nx project scaffold (noop build). |
| domain/entity/asset/package.json | Workspace manifest and scripts for the asset entity package. |
| domain/entity/asset/jest.config.js | Jest config scaffold for the asset entity package. |
| domain/api/assets-data/tsconfig.json | TypeScript config for the new API package. |
| domain/api/assets-data/src/index.ts | Placeholder entrypoint. |
| domain/api/assets-data/README.md | Documents the planned cross-entity contract and invariants for DADA integration. |
| domain/api/assets-data/project.json | Nx project scaffold (noop build). |
| domain/api/assets-data/package.json | Workspace manifest and scripts for the assets-data API package. |
| domain/api/assets-data/jest.config.js | Jest config scaffold for the assets-data API package. |
| .changeset/tidy-pandas-gather.md | Changeset documenting the introduction of the new scaffolded packages. |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 26/27 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
fcc77ff to
64a63a9
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The new React package Jest scaffold includes jest-dom types/deps but doesn’t load jest-dom in setup, and the lockfile includes unrelated churn that should be minimized or split.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
features/platform/assets-data/jest.config.js:5
@testing-library/jest-domis included (devDependency + TS types), but the Jest config doesn't load it viasetupFilesAfterEnv, unlike otherfeatures/platform/*packages (e.g.features/platform/currencies/jest.config.js). Without this, common matchers liketoBeInTheDocument()won't be registered at runtime when tests are added.
module.exports = {
testEnvironment: "jsdom",
roots: ["<rootDir>/src"],
testMatch: ["**/*.test.ts?(x)"],
transform: {
pnpm-lock.yaml:2206
- This lockfile hunk changes
@react-native/babel-presetresolution to include an explicit(@babel/core@...)peer param even though this PR only scaffolds new DDD workspace packages (Jest/SWC/TS) and does not touch any React Native deps. Per repo review guidance, consider splitting unrelated lockfile churn into a separate PR (or regenerate the lockfile in a way that only reflects the new workspaces) to keep the diff reviewable and reduce merge conflicts.
- Files reviewed: 26/27 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Rsdoctor Bundle Diff AnalysisFound 7 projects in monorepo, 1 project with changes. 📊 Quick Summary
📋 Detailed Reports (Click to expand)📁 mobilePath:
📦 Download Diff Report: mobile Bundle Diff Generated by Rsdoctor GitHub Action |
There was a problem hiding this comment.
🟢 Ready to approve
The changes are isolated scaffolding consistent with existing DDD package patterns and introduce no runtime behavior changes.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The changes are limited to consistent scaffolding of new private workspaces plus expected knip/lockfile wiring, with no runtime code movement or behavior changes.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
9fdde6c to
84d2ac5
Compare
|
84d2ac5 to
7d0011c
Compare
7d0011c to
650ddfc
Compare
LucasWerey
changed the title
There was a problem hiding this comment.
🟡 Not ready to approve
The PR claims no runtime-impacting dependency version changes, but pnpm-lock.yaml shows a resolved transitive version change (and there are also package metadata inconsistencies to fix).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (4)
pnpm-lock.yaml:69288
- The lockfile change switches
react-native-qrcode-svg's resolvedqrcodedependency from 1.5.4 to 1.5.3, which is a real runtime version change (not just metadata churn). This contradicts the PR description's "no version changed" / "no runtime impact" claims; either restore the previous resolution or document/justify the downgrade.
pnpm-lock.yaml:69296 - Second instance of the same resolved-version change:
react-native-qrcode-svgnow resolvesqrcodeto 1.5.3 here as well (previously 1.5.4). If this is unavoidable pnpm churn, it should be called out since it affects runtime dependencies.
domain/entity/aggregated-asset/package.json:5 - The package description claims this entity owns "network info and currency ordering", but the README explicitly states
NetworkInfoandCurrenciesOrderdeliberately live in@domain/api-aggregated-assets. Keeping the description aligned avoids misleading tooling/readers when these packages start being used.
"description": "Domain entity for aggregated assets: crypto-asset metadata, network info and currency ordering",
features/platform/aggregated-assets/package.json:5
- The package description still uses the old "assets-data" naming even though the package (and the rest of the PR) is "aggregated-assets". This can create confusion once the workspace list grows and these packages get consumed.
"description": "App-facing assets-data runtime: hooks over the DADA API, cache selectors, asset discovery and currency selection",
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
650ddfc to
56fe24b
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
There are concrete inconsistencies and risk signals to address (API package tsconfig typing conventions, entity package description vs README, and a lockfile dependency resolution change that contradicts the PR’s “no version changes” claim).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (5)
pnpm-lock.yaml:69296
- Second occurrence of the
react-native-qrcode-svgsnapshot also switchesqrcodefrom 1.5.4 to 1.5.3; if this downgrade is unavoidable, please call it out explicitly in the PR description as the PR currently claims no existing dependency versions changed.
domain/entity/aggregated-asset/package.json:5 descriptionclaims this entity package owns network info and currency ordering, but the README explicitly saysNetworkInfoandCurrenciesOrderare intentionally not owned here. This inconsistency will mislead consumers and future maintainers.
"description": "Domain entity for aggregated assets: crypto-asset metadata, network info and currency ordering",
domain/api/aggregated-assets/tsconfig.json:10
- This API package is intended to host RTK Query endpoints and fetch-based request code, but the tsconfig currently omits the
DOMlib andnodetypes. This is inconsistent with otherdomain/api/*packages that use RTK Query (e.g.domain/api/currency-token/tsconfig.json:4-10) and will cause missingfetch/RequestInit/Node global typings once implementation lands.
"compilerOptions": {
"lib": ["ES2022"],
"target": "ES2022",
"module": "ESNext",
"moduleResolution": "bundler",
"noEmit": true,
"types": ["jest"]
},
pnpm-lock.yaml:69288
- Lockfile regeneration changes the resolved
qrcodeversion used byreact-native-qrcode-svgfrom 1.5.4 to 1.5.3. That’s a real dependency resolution change (not just metadata churn) and contradicts the PR description’s claim that no existing package versions changed; please either eliminate this downgrade or explicitly document/justify it.
This issue also appears on line 69292 of the same file.
.changeset/tidy-pandas-gather.md:8
- The PR description’s package/path table references
assets-data(e.g.domain/api/assets-data,features/platform/assets-data), but the actual scaffolded packages here areaggregated-assets/aggregated-asset. Please update the PR description to match the implemented package names/paths to avoid confusion for reviewers and for the follow-up tickets.
Scaffold the DDD packages that will receive the dada-client code: the aggregated-asset and interest-rate entities, the aggregated-assets API client and its app-facing platform layer
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
56fe24b to
5cbc6d1
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The lockfile diff includes an unexpected resolved-version change (qrcode 1.5.4 → 1.5.3 via react-native-qrcode-svg) that contradicts the PR’s “no version changes” claim and should be reverted or explicitly justified.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
domain/entity/aggregated-asset/package.json:5
package.jsondescription says this entity owns “network info and currency ordering”, but the README explicitly statesNetworkInfoandCurrenciesOrderare deliberately not owned by this entity (they live in@domain/api-aggregated-assets). This inconsistency can mislead consumers and future maintainers when the real types land.
"description": "Domain entity for aggregated assets: crypto-asset metadata, network info and currency ordering",
pnpm-lock.yaml:69362
- The lockfile change downgrades
react-native-qrcode-svg’sqrcodedependency from1.5.4to1.5.3, which is a real resolved-version change (not just metadata). This contradicts the PR description claim that no existing package versions changed; please either revert/avoid this resolution change or document/justify it explicitly (since it could affect runtime behavior whereverreact-native-qrcode-svgis used).
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
5cbc6d1 to
7ae1998
Compare
Create the packages that libs/ledger-live-common/src/dada-client will move into
(LIVE-35225, epic LIVE-35223). Scaffolding only: no code moved, no behaviour
change, nothing imports them yet.
@domain/entity-aggregated-asset the network-grouped meta-currency
@domain/entity-interest-rate interest rate shape and its APY type
@domain/api-aggregated-assets DADA wire contract, RTK Query, transforms
@features/platform-aggregated-assets hooks, cache selectors, discovery
Named for the domain concept, not the vendor service: DADA will later back
other domains (dynamic currencies, app search), so it appears in no package
name. 'aggregated-asset' rather than 'asset' because the latter is
indistinguishable from entity-currency*, libs/asset-aggregation and
libs/asset-detail.
The React layer sits in features/platform rather than features/flow because
Market, Portfolio, Global Search and the asset selectors all consume it.
READMEs carry the decisions instead of a docs/ page, since the repo has no ADR
convention: why wire types belong to the api package, why markets stays loosely
typed with no domain/entity/market, the copied dadaIdToMarketId debt, and the
invariants that break with no type error. NetworkInfo and CurrenciesOrder are
explicitly not entities -- a network is a chain already modelled by
entity-currency-crypto, and CurrenciesOrder is server sort metadata.
Registered in knip.json per the born-migrated rule for new packages. The jsdom
package loads @testing-library/jest-dom via jest.setup.js, matching
features/platform/{style,currencies,feature-flags}.
Ref: WXP page 'DADA DDD compliant'.
7ae1998 to
d96324e
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The PR introduces a real transitive dependency version change in the lockfile and has a few scaffold metadata inconsistencies (naming/docs/peers) that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (4)
features/platform/aggregated-assets/package.json:5
- Package description still refers to "assets-data" even though this package is named/located as "aggregated-assets". This is likely leftover naming from the DADA-client and will be confusing once published/consumed.
"description": "App-facing assets-data runtime: hooks over the DADA API, cache selectors, asset discovery and currency selection",
domain/entity/aggregated-asset/package.json:5
- The package.json description claims this entity owns network info and currency ordering, but the README explicitly says
NetworkInfoandCurrenciesOrderlive in@domain/api-aggregated-assets(wire/response metadata). The package description should match the documented ownership to avoid misleading consumers.
"description": "Domain entity for aggregated assets: crypto-asset metadata, network info and currency ordering",
domain/api/aggregated-assets/package.json:28
- All existing
domain/api/*packages declarereactandreact-reduxas peerDependencies (even though they are not regular deps), but this new API package doesn't. Adding the same peerDependencies now will keep the scaffold consistent and avoid having consumers accidentally pull in their own incompatible versions once RTK Query React hooks land.
"devDependencies": {
"@jest/globals": "catalog:",
"@swc/core": "catalog:",
"@swc/jest": "catalog:",
"@types/jest": "catalog:",
pnpm-lock.yaml:69416
- The lockfile re-resolution includes a real transitive version change:
react-native-qrcode-svg@6.1.1now resolvesqrcodeto1.5.3(was1.5.4in the diff). This contradicts the PR description claim that no dependency versions changed and could have runtime impact in QR code generation; please either restore the previous resolution or document/justify this change explicitly.
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
There are verified inconsistencies in new package metadata and an unexpected lockfile dependency resolution change that contradicts the PR’s “no version changes” claim and should be confirmed or corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
pnpm-lock.yaml:69416
- The PR description states that no existing dependency versions changed, but
react-native-qrcode-svgis now resolved againstqrcode: 1.5.3here (previously1.5.4in the diff). Since lockfile churn can introduce real runtime changes, please confirm this downgrade is expected/unavoidable (or regenerate to keep the previous resolution).
domain/entity/aggregated-asset/package.json:5 package.jsondescription says this entity owns “network info and currency ordering”, but the README explicitly statesNetworkInfoandCurrenciesOrderdeliberately live in@domain/api-aggregated-assets(wire type / response metadata). This mismatch will get stale quickly and can mislead consumers when searching packages.
"description": "Domain entity for aggregated assets: crypto-asset metadata, network info and currency ordering",
features/platform/aggregated-assets/package.json:5
- The package is named
@features/platform-aggregated-assets, but itsdescriptionstill says “assets-data runtime”. Since this string shows up in tooling (workspace listings, IDE search, etc.), keeping it aligned with the actual package name avoids confusion during the migration.
"description": "App-facing assets-data runtime: hooks over the DADA API, cache selectors, asset discovery and currency selection",
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
✅ Checklist
npx changesetwas attached.typecheckandjesttargets are verified to run, which is all there is to assert at this stage. Code and its tests arrive in LIVE-35226 / LIVE-35227.- No runtime impact. New private workspace packages, all empty (
export {};). No existing source file is modified — onlyknip.json(+12) andpnpm-lock.yaml.- Nothing imports these packages yet, so no app behaviour can change.
- No QA pass needed.
📝 Description
Problem.
libs/ledger-live-common/src/dada-clientis being migrated into the DDD layers under LIVE-35223. The relocation itself (LIVE-35226 / LIVE-35227) is a large diff, so the destination packages are created first, empty, in their own reviewable step.Solution. Scaffold the packages per the
docs/new-library.mdchecklist. No code moved.domain/entity/aggregated-asset@domain/entity-aggregated-assetCryptoAssetMeta— the network-grouped meta-currencydomain/entity/interest-rate@domain/entity-interest-rateInterestRate,ApyTypedomain/api/aggregated-assets@domain/api-aggregated-assetsfeatures/platform/aggregated-assets@features/platform-aggregated-assetsEach gets
package.json,tsconfig.json,jest.config.js,project.json,README.mdwith a status marker, and a placeholdersrc/index.ts. The jsdom package also getsjest.setup.jsloading@testing-library/jest-dom.Design decisions were reviewed with @gre and Yoann and are written up on the Confluence page DADA DDD compliant. Summary of what shaped this PR:
aggregated-assets, notdada— DADA will later back other domains (dynamic currency fetching, app search), which get their own api packages. The service name appears nowhere.aggregated-asset, notasset. The latter is indistinguishable fromentity-currency*,libs/asset-aggregationandlibs/asset-detail. The concept this owns is a meta-currency grouping several per-network currencies viaassetsIds, which resolves more than the lower-level currency / token / fiat types.features/platform, notfeatures/flow. Market, Portfolio, Global Search and the asset/network selectors all consume it, so it is a capability shared across flows rather than one journey's internals.NetworkInfoandCurrenciesOrderare explicitly not entities. A network is a chain, already modelled byentity-currency-crypto, so it stays a wire type resolving to that.CurrenciesOrderis{ key, order, metaCurrencyIds }— server sort metadata belonging to the api response contract. Both READMEs say so.marketsstays loosely typed — nodomain/entity/market. It would roughly double the migration and pull in another team's surface, andPartial<MarketItemResponse>has nothing meaningful to validate.docs/page, because this repo has no ADR convention (nothing indocs/about-docs,docs/contributing,AGENTS.mdor thedocs-update-or-createskill). A reader of the package finds the rationale without needing Jira or Confluence.knip.json(+12, oneworkspacesentry each) per the knip-migration skill's "new package = born migrated" rule. Exports are explicit with no./*wildcard. Verified:pnpm run unimportedruns clean.Notes for reviewers
0.1.0on all packages, matching themarket-sentimentprecedent where the creating PR carried aminorchangeset (0.1.0→0.2.0).testscripts use--passWithNoTestsso empty packages don't fail their targets. Remove when real tests land in LIVE-35226 / LIVE-35227.zod,@reduxjs/toolkit,@shared/envand the currency entities are added by the PR that moves the code in, so this one doesn't declare deps nothing imports.@features/platform-aggregated-assetsdoes carryreact/react-reduxpeers, matchingfeatures/platform/currencies.pnpm-lock.yamlshows ~35 deleted lines, all of which are pnpm reordering existing importer blocks, not removals. Verified:wallet-sync,starred-account,cloud-syncandcloud-sync-moduleare all still present with their dependencies intact, and the importer line count is exactly +4 — this PR's four packages. Regenerated with the pinnedpnpm@10.24.0from a clean base.nx show projectslists all four, knip clean,commitlint --from origin/developpasses.Copilot review, addressed
jsdomand its tsconfig declared@testing-library/jest-domtypes, but nothing loaded the matchers, so a test usingtoBeInTheDocumentwould have compiled then failed. Fixed; confirmed with a throwaway probe test before and after. Now matchesfeatures/platform/{style,currencies,feature-flags}.packageManagerpinspnpm@10.24.0and that is the version used, so it is not a wrong-version artifact.🔎 Known open item
This PR ships four packages. The Confluence page proposes folding
entity/interest-rateintoentity-aggregated-assetfor three — a rate is only ever attached to an aggregated asset and nothing fetches rates independently. That is a package deletion rather than part of the scaffold, so it is deliberately not in scope here. Happy to fold it in before merge if reviewers prefer.❓ Context
developdirectly.🧐 Checklist for the PR Reviewers